DM-38546: fixes while implementing new CalibrateImageTask #181
Conversation
parejkoj
force-pushed
the
tickets/DM-38546
branch
from
43110e4
to
e323d22
Compare
python/lsst/meas/astrom/ref_match.py
Outdated
| sourceSelector = sourceSelectorRegistry.makeField( | ||
| doc="How to select sources for cross-matching.", | ||
| default="science", | ||
| optional=True |
There was a problem hiding this comment.
I'm normally in favor of adding doX options since there's such a strong precedent for it, but in this case setting the registry field selection to None seems to express the desired behavior plenty well enough, and unless we add something to validate to check doSourceSelection is False if and only if sourceSelector is None, we're messing up configuration comparisons a bit (though surely not for the first time), by making it so two functionally-equivalent configs with different sourceSelector values but doSourceSelection=False compare as unequal.
It's a pity the DirectMatchConfigWithoutLoader.sourceSelector field is a ConfigurableField and can't be optional, though, and consistency is also desirable.
What do you think about having a do-nothing SourceSelector implementation instead of any of these config field changes?
There was a problem hiding this comment.
I've added a NullSourceSelectorTask to meas_algorithms and reverted this change.
Conceptually, I don't like it because you should be able to truly do nothing, not even create a numpy array of bools, but that's probably a small enough thing to not be a concern in practice. But yes, I do wish we could just set those registry fields to None and be done with it; revising our uses of them to all use optional registries would be a nice deprecation step, but might be hard to do in practice.
| def __init__(self, refObjLoader, schema=None, **kwargs): | ||
| RefMatchTask.__init__(self, refObjLoader, **kwargs) | ||
| def __init__(self, refObjLoader=None, schema=None, **kwargs): | ||
| RefMatchTask.__init__(self, refObjLoader=refObjLoader, **kwargs) |
There was a problem hiding this comment.
It'd be nice to actually deprecate this argument, but certainly out of scope for this ticket.
There was a problem hiding this comment.
I'm not sure we actually want to deprecate it: I could see uses of it in e.g. notebooks.
My test data had only 3 points, which resulted in a confusing error inside _get_pair_pattern_statistics(); this at least makes it obvious when we're in a state that would cause trouble later.
We don't want to do any source selection in the new calibrateImageTask, as we are selecting sources directly in that task; astrometry and directMatch source selectors are now optional.
Default config field already has this set.
This was a purely gen2 kwarg.
gen3 pipetasks call setRefObjLoader; users in notebooks and tests may still want to use the __init__ interface though.
parejkoj
force-pushed
the
tickets/DM-38546
branch
from
e323d22
to
8e9d954
Compare
This reverts commit efacb34.
No description provided.